Skip to content

[SPARK-4166][Core][WebUI] Display the executor ID in the Web UI when ExecutorLostFailure happens #3033

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-4166][Core][WebUI] Display the executor ID in the Web UI when ExecutorLostFailure happens #3033

wants to merge 2 commits into from
+13 −8

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Oct 31, 2014

Now when ExecutorLostFailure happens, it only displays ExecutorLostFailure (executor lost). Adding the executor id will help locate the faulted executor.

@zsxwing zsxwing changed the title Display the executor ID in the Web UI when ExecutorLostFailure happens [SPARK-4166][Core][Web UI] Display the executor ID in the Web UI when ExecutorLostFailure happens Oct 31, 2014
@zsxwing zsxwing changed the title [SPARK-4166][Core][Web UI] Display the executor ID in the Web UI when ExecutorLostFailure happens [SPARK-4166][Core][WebUI] Display the executor ID in the Web UI when ExecutorLostFailure happens Oct 31, 2014
@SparkQA
Copy link

SparkQA commented Oct 31, 2014

Test build #22603 has started for PR 3033 at commit c5c4cf2.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Oct 31, 2014

Test build #22603 has finished for PR 3033 at commit c5c4cf2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorLostFailure(execId: String) extends TaskFailedReason

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22603/
Test PASSed.

@JoshRosen
Copy link
Contributor

This looks good to me.

@JoshRosen
Copy link
Contributor

Actually, there might be a backwards-compatibility concern here when event logs written by older Spark versions. Ping @andrewor14 for review.

@andrewor14
Copy link
Contributor

Hey @zsxwing can you attach a screenshot of what it looks like before and after?

andrewor14
andrewor14 reviewed Oct 31, 2014
View reviewed changes
core/src/main/scala/org/apache/spark/util/JsonProtocol.scala
@@ -636,7 +638,9 @@ private[spark] object JsonProtocol {
new ExceptionFailure(className, description, stackTrace, metrics)
case `taskResultLost` => TaskResultLost
case `taskKilled` => TaskKilled
case `executorLostFailure` => ExecutorLostFailure
case `executorLostFailure` =>
val executorId = (json \ "Executor ID").extract[String]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this needs to be backward compatible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this can just be extractOpt with a default executor ID of "unknown"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a Utils.jsonOption that we can use here

@JoshRosen
Copy link
Contributor

@andrewor14 I think #3032 is the web UI display half of this PR, which contains screenshots; this looks like it's just adding additional information to ExecutorLostFailure.

@andrewor14
Copy link
Contributor

Oh I see, this patch doesn't make any changes to the UI itself

@SparkQA
Copy link

SparkQA commented Nov 1, 2014

Test build #22680 has started for PR 3033 at commit ff4664c.

  • This patch merges cleanly.

@zsxwing
Copy link
Member Author

zsxwing commented Nov 1, 2014

I updated to use Utils.jsonOption to support backward compatible. Also update #3032

@SparkQA
Copy link

SparkQA commented Nov 1, 2014

Test build #22680 has finished for PR 3033 at commit ff4664c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class ExecutorLostFailure(execId: String) extends TaskFailedReason

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22680/
Test PASSed.

@JoshRosen
Copy link
Contributor

This looks good to me; I compared this against other recent changes to JsonProtocol and it seems to handle backwards-compatibility in the proper way, so I'm going to merge it. Thanks!

@asfgit asfgit closed this in 4e6a7a0 Nov 2, 2014
@zsxwing zsxwing deleted the SPARK-4166 branch November 3, 2014 02:02
@andrewor14
Copy link
Contributor

Hey @zsxwing actually can you add a backward compatibility test in JsonProtocolSuite? We actually require a test suite if people want to change the signature of the relevant classes.

@zsxwing
Copy link
Member Author

zsxwing commented Nov 4, 2014

Already added it in #3085

@zsxwing zsxwing mentioned this pull request Nov 4, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zsxwing @SparkQA @AmplabJenkins @JoshRosen @andrewor14